Add improved support for signals (raise, kill, sigaction, etc) #14883
Conversation
sbc100
force-pushed
the
signals
branch
5 times, most recently
from
a298ab1 to
6ab435a
Compare
sbc100
force-pushed
the
signals
branch
3 times, most recently
from
d20530c to
e582eb4
Compare
sbc100
force-pushed
the
signals
branch
2 times, most recently
from
c4bdb57 to
cfd9520
Compare
Although we don't have any support for external or async signal a lot of this functionality works fine within a single process (e.g. using raise the syncronously run signal handlers in your own process). I'm adding this so that as a followup we can implement `pthread_kill` to send signal between threads. See: #14872
|
|
||
| return 0; | ||
| } | ||
| // Avoid a direct call to the handler, and instead call via JS so we can |
There was a problem hiding this comment.
should we add a test with a mismatched signature below, or is posixtestsuite sufficient?
There was a problem hiding this comment.
I think we can call posixtestsuite enough.
| - Added some support for signal handling libc functions (raise, kill, | ||
| sigaction, sigpending, etc). We still don't have a way to deliver signals from | ||
| the outside but these at least now work for sending signals to the current | ||
| thread (JS context) (#14883). |
There was a problem hiding this comment.
This changelog note should probably be moved to 2.0.28 (since 2.0.27 was already released).
There was a problem hiding this comment.
Oops, I guess we never mark 2.0.27 as released: #14898
…ipten-core#14883) Although we don't have any support for external or async signal a lot of this functionality works fine within a single process (e.g. using raise the syncronously run signal handlers in your own process). I'm adding this so that as a followup we can implement `pthread_kill` to send signal between threads. See: emscripten-core#14872
| @@ -259,7 +259,7 @@ int sigandset(sigset_t *, const sigset_t *, const sigset_t *); | |||
|
|
|||
| #define SIG_ERR ((void (*)(int))-1) | |||
| #define SIG_DFL ((void (*)(int)) 0) | |||
| #define SIG_IGN ((void (*)(int)) 1) | |||
| #define SIG_IGN ((void (*)(int))-2) /* XXX EMSCRIPTEN: use -2 since 1 is a valid function address */ | |||
There was a problem hiding this comment.
This change seems to be a regression for Python. Some code somewhere must assume that SIG_IGN is 1.
There was a problem hiding this comment.
Reverting this could be tricky because, as the comment says, 1 is a valid function point in Wasm.. so comparing and incoming function pointer with SIG_IGN could produce false positives.
Can you find the fix the code that contains that assumption? ..
Could it be that not all object files were re-built?
There was a problem hiding this comment.
Yeah the rational for the change is clear. I am trying to hunt down what is going on. The error reproduces in our CI so it's definitely from a clean build. But we could patch it back in Pyodide if need be.
@tiran Have you run into this problem? Is there a Python patch we can backport?
There was a problem hiding this comment.
As a last resort we could shift the range of valid functions pointers from a range that starts at 1 to range that starts at something higher, such as 16, but it seems like quite in intrusive change (and not 100% free at runtime)... so I would much rather find an fix offending code.
There was a problem hiding this comment.
It looks to me like the Python signalmodule code has a bug that is fixed in v3.11: in v3.10 in compares the memory addresses of the Python objects, and it's returning False because they are two different -2's. I suspect the Python interpreter interns 1 in some special way which makes the wrong code work by chance when the value in question is 1.
There was a problem hiding this comment.
There was a problem hiding this comment.
Okay I will apply the patch in @tiran's comment there.
There was a problem hiding this comment.
Yeah, I ran into the problem, too. PR python/cpython#31759 fixed the issue in 3.11 / main. I did not backport the fix to 3.10.
There was a problem hiding this comment.
Backports cleanly though. Thanks @tiran it's interesting rediscovering your work like this.
Although we don't have any support for external or async signal a lot of
this functionality works fine within a single process (e.g. using raise
the syncronously run signal handlers in your own process).
I'm adding this so that as a followup we can implement
pthread_killto send signal between threads. See: #14872